Skip to content

Added validation rule for apiMethod parameter in getTreemapData API call, #AS-494#72

Merged
AltamashShaikh merged 8 commits into5.x-devfrom
AS-494
Feb 14, 2026
Merged

Added validation rule for apiMethod parameter in getTreemapData API call, #AS-494#72
AltamashShaikh merged 8 commits into5.x-devfrom
AS-494

Conversation

@AltamashShaikh
Copy link
Copy Markdown
Contributor

@AltamashShaikh AltamashShaikh commented Feb 12, 2026

Description

Added validation rule for apiMethod parameter in getTreemapData API call

Issue No

#AS-494

Steps to Replicate the Issue

  1. It should block requests like below
/index.php?module=API&action=index&method=TreemapVisualization.getTreemapData&idSite=6&period=day&date=2026-01-01&apiMethod=SitesManager.deleteSite&column=something

Checklist

  • [✔] Tested locally or on demo2/demo3?
  • [✖] New test case added/updated?
  • [✔] Are all newly added texts included via translation?
  • [NA] Are text sanitized properly? (Eg use of v-text v/s v-html for vue)
  • [✔] Version bumped?
  • [NA] I have understood, reviewed, and tested all AI outputs before use
  • [NA] All AI instructions respect security, IP, and privacy rules

@AltamashShaikh AltamashShaikh requested review from a team and sgiehl February 12, 2026 12:51
sgiehl
sgiehl requested changes Feb 12, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only roughly looked through code. Haven't done any testing.

james-hill-matomo
james-hill-matomo reviewed Feb 13, 2026
View reviewed changes
james-hill-matomo
james-hill-matomo suggested changes Feb 13, 2026
View reviewed changes
API.php Outdated
list($apiName, $apiAction) = explode('.', $apiMethod);
$disAllowedApiActions = ['getBulkRequest'];
// Block if API action does not start with get
if (!in_array($apiAction, $disAllowedApiActions) || stripos($apiAction, 'get') !== 0) {
Copy link
Copy Markdown

@james-hill-matomo james-hill-matomo Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That first ! is incorrect right?

Some quick functional tests would catch & clarify this.

lachiebol
lachiebol reviewed Feb 13, 2026
View reviewed changes
lachiebol
lachiebol previously approved these changes Feb 13, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@lachiebol lachiebol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, getBulkRequest & anything that doesn't start with get is returning an error

james-hill-matomo
james-hill-matomo previously approved these changes Feb 13, 2026
View reviewed changes
Copy link
Copy Markdown

@james-hill-matomo james-hill-matomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in dev and it still worked OK.

Applying CSRF protections would be nice. I manually tested the correct methods are blocked.

@AltamashShaikh
Copy link
Copy Markdown
Contributor Author

AltamashShaikh commented Feb 13, 2026

Tested in dev and it still worked OK.

Applying CSRF protections would be nice. I manually tested the correct methods are blocked.

@james-hill-matomo CSRF for API ? Can you explain a bit ?

@AltamashShaikh
Copy link
Copy Markdown
Contributor Author

AltamashShaikh commented Feb 13, 2026

@james-hill-matomo Your approval is needed to merge the code.

james-hill-matomo
james-hill-matomo approved these changes Feb 13, 2026
View reviewed changes
Copy link
Copy Markdown

@james-hill-matomo james-hill-matomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(:

@AltamashShaikh AltamashShaikh merged commit 64f1379 into 5.x-dev Feb 14, 2026
8 checks passed
@AltamashShaikh AltamashShaikh deleted the AS-494 branch February 14, 2026 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sgiehl sgiehl sgiehl approved these changes

@lachiebol lachiebol lachiebol approved these changes

+1 more reviewer

@james-hill-matomo james-hill-matomo james-hill-matomo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AltamashShaikh @sgiehl @lachiebol @james-hill-matomo